RSDK-9298: Instance class #374

lia-viam · 2025-02-25T20:49:17Z

Introduces a one-and-only Instance class for the SDK. For the time being this only has a Registry, but it will pave the way for inclusion of better logging and resource-level logging in particular.

Some of the machinery in instance.cpp for singleton management was adapted from https://github.com/mongodb/mongo-cxx-driver/blob/02ad73bc306eb9044eb691de96ad41c13107738d/src/mongocxx/lib/mongocxx/v_noabi/mongocxx/instance.cpp

A few classes were updated to now get a pointer to a Registry rather than calling the no-longer-static Registry methods. The existence of Instance::current() means technically these could just get the Registry from the current instance, or possibly that Registry could have a static Registry& current() method...open to feedback here.

Every example has been updated to define an instance object, with the same boilerplate comment put in front explaining its usage

…stry-member

lia-viam · 2025-02-26T17:58:14Z

src/viam/sdk/module/service.cpp

 std::shared_ptr<Resource> ModuleService::get_parent_resource_(const Name& name) {
    if (!parent_) {
-        parent_ = RobotClient::at_local_socket(parent_addr_, {0, boost::none});
+        parent_ = RobotClient::at_local_socket(parent_addr_, {0, boost::none}, registry_);


Question for reviewers--I am wondering if RobotClient should be part of the Instance as well, it seems like we always want it? Looking through the examples I believe the only scenario where RobotClient is not constructed explicitly is one where we just construct a ModuleService but that ends up constructing a RobotClient in this method anyway.

Looking ahead to the logger PR, the logger will need a RobotClient for the gRPC Log call, so we may want to avoid a situation where RobotClient is getting delayed instantiation and just construct one on application startup.

I think I'd prefer to keep RobotClient distinct.

I think of the Instance as being a bit of a black box; the user knows they need one and that they need to keep it around, but they don't need to know anything about what's inside. By contrast, a RobotClient is something that users may interact with directly and frequently.

There should only ever be one live instance, whereas one could imagine a user writing client code that interacts with multiple robots and so therefore has multiple robot clients. We could have some sort of map from name to RobotClient inside the instance but then a user starts needing to know about Instance APIs and we're adding new layers of abstraction between the user and their access to robot APIs and I think it just starts to get a bit complicated.

Those points make sense. Follow-ups

if there are multiple RobotClient should log messages be getting sent to the Log call of each of them?

given the comment about Instance insulation, how do we feel about now having to do inst.registry() for some of the constructors?

Hmm good questions!

if there are multiple RobotClient should log messages be getting sent to the Log call of each of them?

I think this is a question we can largely ignore, thankfully. If there are multiple RobotClients then the user is interacting with purely client code and so the logs should be showing up in the shell, so we dont have to call a RobotClient::log at all.

If on the other hand, we do have to send logs via the Log method then we're dealing with a module. But, each module is started up as its own process for a single Robot, so we can always only send logs via the Log call to the module's parent.

given the comment about Instance insulation, how do we feel about now having to do inst.registry() for some of the constructors?

Caveat: I will readily confess to not being particularly hip to C++ conventions. If what I'm saying flies in the face of idiomatic C++ design then obviously we should prefer remaining idiomatic.

I personally prefer to not do that. Per our offline conversation yesterday and to reiterate some of that: I'd love to hide as much as possible from users about the instance, and ideally not really expose any methods at all to the user about it. If it's needed in particular cases, I'd prefer to just pass the instance as an argument and let the RobotClient constructor, e.g., deal with getting the Registry from a private method via some friend class setup.

I don't think this is always clean and easy to do, as when registering custom resources, but I imagine we could add a register_resource method to a RobotClient that under the hood interacts with the registry. I think this is a bit cleaner and a bit more intuitive (when you want a robot client to deal with a custom resource, you register that resource with the client!). A possible downside here is this pattern is slightly misleading: we're saying "register it with the robot client" but in fact we're registering with the singleton registry, which means another robot client would also be able to work with that resource type. But I don't think this is particularly harmful, and we can work around it if it is.

Ah that's helpful--in that case I can have a log backend which binds to a RobotClient but only register it with the logging core when that RobotClient is the parent of a ModuleService !

As for the other stuff I've made some further updates which I think clean up the usage a lot, using the current method as discussed yesterday

stuqdog

Nice, this is super exciting! Thanks so much for putting this together, it's a complicated one but a big win.

I've done a first pass; there's a lot to think about here so I think there will be more back and forth but here are some initial thoughts.

src/viam/examples/modules/simple/client.cpp

src/viam/examples/modules/simple/main.cpp

stuqdog · 2025-02-28T18:11:44Z

src/viam/sdk/robot/client.hpp

    status get_machine_status() const;

   private:
+    Registry* registry_;


(minor) I don't feel too strongly, but is this change necessary? It seems to me that we could just use Registry::get() in the refresh() call rather than holding onto this pointer. It might be marginally less performant to have to get the Registry every time we refresh, but I don't expect it's a significant cost and it helps simplify the info that a user is exposed to in the headers.

Maybe cc @acmorrow when he gets a sec, but wondering if Registry can just get friend access to instance and be able to return a reference to the registry member rather than having to access it through Instance::current with the atomic::loads. My though is that since we say it's UB to use any Viam types without an Instance already constructed we can skip the whole "create if not exists" logic of Instance::current

I agree that the performance penalty can probably be ignored here, but if so then we could do

static Registry& Registry::get() { static Registry& result = Instance::current().impl_->registry; return result; }

in which case the static would only be initialized one-and-done style

stuqdog · 2025-02-28T18:13:25Z

src/viam/examples/modules/tflite/main.cpp


    // Construct the module service and tell it where to place the socket path.
-    auto module_service = std::make_shared<vsdk::ModuleService>(socket_path);
+    auto module_service = std::make_shared<vsdk::ModuleService>(socket_path, inst.registry());


I think this change is out of date? It doesn't look like the ModuleService takes a registry as part of its constructor.

Oops yes good catch, tflite is not built in CI and hadn't rebuilt it locally

stuqdog · 2025-02-28T18:20:19Z

src/viam/sdk/common/instance.hpp

+/// @brief Instance management for Viam C++ SDK applications.
+/// This is a single instance class which is responsible for global setup and teardown related to
+/// the SDK. An Instance must be constructed before doing anything else in a program, and it must
+/// remain alive in a valid state for the duration of the program. Creating multiple overlapping


How difficult would it be to get rid of the overlapping clause and just make it an error to create multiple instances, period?

When we create an instance we create a new registry and run a second registry::initialize(), which sets up gRPC reflection a second time. I could easily imagine this getting pretty complicated and having unforeseen side effects. It's definitely an edge case, but I think just being upfront that users can only ever create one instance helps simplify things.

There's no difficulty per se, we can just tell people "don't ever do this, it's UB" 🙂

stuqdog · 2025-02-28T18:28:08Z

src/viam/sdk/module/service.cpp


-        const std::shared_ptr<const ModelRegistration> reg = Registry::lookup_model(cfg.name());
+        const std::shared_ptr<const ModelRegistration> reg =
+            parent.registry_->lookup_model(cfg.name());


see above re: parent.registry_

stuqdog · 2025-02-28T18:28:20Z

src/viam/sdk/module/service.cpp


        const std::shared_ptr<const ModelRegistration> reg =
-            Registry::lookup_model(cfg.api(), cfg.model());
+            parent.registry_->lookup_model(cfg.api(), cfg.model());


see above re: parent.registry_

stuqdog · 2025-02-28T18:29:24Z

src/viam/sdk/module/service.hpp

                                   std::string const& resource_name);
    std::shared_ptr<Resource> get_parent_resource_(const Name& name);

+    Registry* registry_;


see above comments on this being a class data member.

stuqdog · 2025-02-28T18:49:22Z

src/viam/sdk/registry/registry.hpp

-    static std::unordered_map<API, std::shared_ptr<const ResourceClientRegistration>> client_apis_;
-    static std::unordered_map<API, std::shared_ptr<const ResourceServerRegistration>> server_apis_;
+    mutable std::mutex lock_;
+    bool initialized_{false};


(minor) I think we can get rid of this entirely now. Now that we have a single instance of a registry that gets automatically initialized when the Instance object is created, we don't need to track this anymore.

stuqdog · 2025-02-28T19:13:58Z

src/viam/sdk/common/instance.cpp

+    if (current == reinterpret_cast<Instance*>(&sentinel)) {
+        throw Exception("instance was destroyed");
+    }


(q) pardon my ignorance, I don't think I fully understand this. Under what conditions will this exception throw?

Oh wait actually this gets to your comment above about making it an error to have multiple instances even if non-overlapping, so it already is an error to have multiple of them

Co-authored-by: Ethan <[email protected]>

…stry-member

…to registry-member

acmorrow · 2025-03-05T21:23:47Z

src/viam/sdk/common/instance.hpp

+    Instance();
+    ~Instance();
+
+    static Instance& current();


Do you want to doc-comment this, particularly if it is mostly intended for testing.

acmorrow · 2025-03-05T21:26:15Z

src/viam/sdk/registry/registry.cpp

 };

+Registry& Registry::get() {
+    static Registry& result = Instance::current().impl_->registry;


This will bring Instance into existence even if one wasn't created, I think. Those maybe aren't the right semantics, since then a user of the library could get away with not creating an Instance themselves.

Why the static& rather than just calling Instance::current() each time.

I think you're right that those aren't the semantics we want--maybe it should be something like Instance::current(bool create_if_not_exists = false)

This was the result of a discussion me and @stuqdog were going back and forth on, and we thought that Instance should be a total black box so that it's in some sense an implementation detail that Instance has-a Registry or, later, Logging-Core. In either case though, I think that the divergences from our use cases and Mongo's mean that Instance::current does not end up being unit tests only. But from the "black box" perspective, the user doesn't know that Instance has a Registry, they just know 1) I have to create an Instance before doing anything, and 2) I can only access an instance of Registry through its static get so I can't create them as I please

acmorrow · 2025-03-05T21:27:06Z

src/viam/sdk/registry/registry.cpp

    grpc::reflection::InitProtoReflectionServerBuilderPlugin();
 }

-std::unordered_map<std::string, std::shared_ptr<const ModelRegistration>> Registry::resources_;


acmorrow · 2025-03-05T21:27:28Z

src/viam/sdk/registry/registry.hpp

 class Registry {
   public:
+    /// @brief Get the application-wide instance of Registry.
+    static Registry& get();


Or instance? I also wonder a bit why to expose this on Registry rather than on Instance.

Fair question--elaborated in another comment

…stry-member

stuqdog

nice, a couple nits but otherwise lgtm!

stuqdog · 2025-03-12T18:53:46Z

src/viam/sdk/common/instance.hpp

+    ~Instance();
+
+    /// @brief Get the current Instance according to the Creation behavior.
+    /// Calling current(Creation::open_existing) when an instance has not yet been constructed is an


(minor) I'd put the error case in a @throws line for clearer doxygen docs.

stuqdog · 2025-03-12T19:04:08Z

src/viam/sdk/common/instance.cpp

+#include <viam/sdk/common/private/instance.hpp>
+#include <viam/sdk/registry/registry.hpp>
+
+#include <atomic>


(nit) can we put this include block above the viam/sdk include block?

lia-viam added 10 commits February 18, 2025 11:02

initial commit of instance object

31437c3

wip: initial commit to get sdk building with instance

932ebaa

add missing implementation

04d0158

update examples to use instance

a105cef

only instance can construct registry

403f316

make tests work with registry member approach

27eeb13

pimpl registry and update examples

ce2aa8c

add instance management infra

e5cf6d0

Merge branch 'main' of github.com:viamrobotics/viam-cpp-sdk into regi…

85194da

…stry-member

update tflite module

5024d3a

lia-viam requested review from acmorrow and stuqdog February 25, 2025 20:49

lia-viam requested a review from a team as a code owner February 25, 2025 20:49

lia-viam requested review from purplenicole730 and removed request for a team February 25, 2025 20:49

lia-viam added 3 commits February 25, 2025 15:59

silence spurious const warning

ef8ec8e

make method static

16113fd

Merge branch 'main' of github.com:viamrobotics/viam-cpp-sdk into regi…

216e5e2

…stry-member

lia-viam commented Feb 26, 2025

View reviewed changes

lia-viam added 4 commits February 27, 2025 13:48

use static current to populate registry member

4771390

revert and adapt examples

18d26c9

make instance more of a black box

6018323

add impl instance to git

5535616

stuqdog reviewed Feb 28, 2025

View reviewed changes

lia-viam and others added 6 commits March 3, 2025 10:05

Add instance comment boilerplate

eeac195

Co-authored-by: Ethan <[email protected]>

Add instance comment boilerplate

7560f6d

Co-authored-by: Ethan <[email protected]>

remove old ctor arg

4e25d35

prohibit multi instance

55baa6c

remove initialized flag

10cd711

meyers singleton registry

c745422

lia-viam added 4 commits March 3, 2025 15:13

remove registry member and fix member spacing/ordering

75ab450

remove unused include

fc21c35

Merge branch 'main' of github.com:viamrobotics/viam-cpp-sdk into regi…

1758ee9

…stry-member

Merge branch 'registry-member' of github.com:lia-viam/viam-cpp-sdk in…

cd1d8c5

…to registry-member

acmorrow reviewed Mar 5, 2025

View reviewed changes

lia-viam added 8 commits March 12, 2025 11:31

Merge branch 'main' of github.com:viamrobotics/viam-cpp-sdk into regi…

6e154ce

…stry-member

reorder includes

f02ce0f

conditionally find unit test framework and link it to tests

e80bf80

registry get cannot create instance by default

185121a

instance models fixture

7a747ec

create if needed instance in fixture

9c4dc97

Merge branch 'main' of github.com:viamrobotics/viam-cpp-sdk into regi…

30fe47e

…stry-member

revert namespace ws

7409dc7

lia-viam requested review from acmorrow and stuqdog March 12, 2025 18:08

stuqdog approved these changes Mar 12, 2025

View reviewed changes

lia-viam added 2 commits March 13, 2025 13:02

code review: update doc comment

c4aaadf

code review: include order

d2ae2d7

lia-viam merged commit 9dc2143 into viamrobotics:main Mar 13, 2025
4 checks passed

lia-viam deleted the registry-member branch March 13, 2025 18:54

lia-viam mentioned this pull request Mar 14, 2025

RSDK-5986: Resource-level logging for the C++ SDK #383

Merged

RSDK-9298: Instance class #374

RSDK-9298: Instance class #374

Uh oh!

Conversation

lia-viam commented Feb 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuqdog Feb 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuqdog left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lia-viam Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuqdog left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

stuqdog Feb 26, 2025 •

edited

Loading

lia-viam Mar 11, 2025 •

edited

Loading